Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove ipld legacy and format globals in ipld/merkledag #322

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

aschmahmann
Copy link
Contributor

Related to #291

@aschmahmann aschmahmann requested a review from a team as a code owner May 30, 2023 20:02
legacy.RegisterCodec(cid.Raw, basicnode.Prototype.Bytes, RawNodeConverter)
// TODO: Don't require global registries
func init() {
d := legacy.NewDecoder()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willscott WDYT about explicitly passing the go-ipld-prime linksystem here instead of requiring the default? This might make it easier for people to get out of the "global defaults" game.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to have this as a global versus the approach i took in ipfs/go-merkledag#104 and making the decoder in the NewDAGService below?

Copy link
Contributor Author

@aschmahmann aschmahmann May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially it was just a stylistic preference that I had since a) it's still a global in 104, it's just a little more subtle b) it's a bit of a DRY thing where I didn't want to accidentally change one instead of the other.

However, I'm now also thinking that given we still have this global we might want to make it settable until we can make relying on a merkledag global registry no longer a thing (especially since it also implicitly hides the go-ipld-prime global registry)

Comment on lines +26 to +28
d.RegisterCodec(cid.DagProtobuf, dagpb.Type.PBNode, ProtoNodeConverter)
d.RegisterCodec(cid.Raw, basicnode.Prototype.Bytes, RawNodeConverter)
ipldLegacyDecoder = d
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willscott thanks for doing ipfs/go-merkledag#104. I mostly copied it. Although I left the globals up in the global space as a reminder for us to try and exit globals land when it becomes possible 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a reminder for us to try and exit globals land when it becomes possible

I'm confused by this - in 104 i did exit globals land. - is there a reason not to stay consistent with that in the copy here, since it's the direction we want to go anyway?

Copy link
Contributor Author

@aschmahmann aschmahmann May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this - in 104 i did exit globals land

We're still stuck with globals:

  1. The merkledag libraries still have globals. IMO this is bad for the same reasons the other ones were bad, it just turns out the other ones are basically unused independently so for now we can at fairly low cost coalesce here (and solve a problem we currently have)
  2. There are still go-ipld-prime globals used implicitly in go-ipld-legacy unless we fix that https://github.com/ipfs/go-ipld-legacy/pull/12/files#r1211104340
    • Ran out of time to do the kubo PR exercising this (chore: update boxo to version with fewer globals kubo#9907). Maybe it'll be fine to leave this merkledag PR mostly alone (aside from passing in something like a linksystem to go-ipld-legacy). However, to start wrestling control away from the globals we might also want to make the legacy decoder used by merkledag publicly settable (albeit with the various warnings about being careful about using it, and trying to only use it in top-level binaries).

is there a reason not to stay consistent with that in the copy here

If we go with making the go-ipld-legacy decoder publicly settable (and therefore not tied to go-ipld-prime's global registry) then this approach looks much better rather than just being a stylistic difference. If it's just a stylistic difference then 🤷.

@aschmahmann aschmahmann force-pushed the feat/remove-ipld-legacy-and-format-globals branch from f87feff to 9a14e5e Compare May 30, 2023 20:10
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #322 (3200229) into main (0927be4) will increase coverage by 0.41%.
The diff coverage is 61.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   49.26%   49.67%   +0.41%     
==========================================
  Files         280      283       +3     
  Lines       33850    33873      +23     
==========================================
+ Hits        16675    16828     +153     
+ Misses      15398    15287     -111     
+ Partials     1777     1758      -19     
Impacted Files Coverage Δ
bitswap/bitswap.go 37.20% <ø> (ø)
bitswap/client/client.go 84.58% <ø> (+2.37%) ⬆️
bitswap/client/internal/getter/getter.go 64.10% <ø> (ø)
...tswap/client/internal/messagequeue/messagequeue.go 69.70% <ø> (+5.14%) ⬆️
bitswap/client/internal/peermanager/peermanager.go 65.41% <ø> (ø)
...ernal/providerquerymanager/providerquerymanager.go 64.73% <ø> (ø)
bitswap/client/internal/session/session.go 82.18% <ø> (ø)
.../internal/sessionpeermanager/sessionpeermanager.go 72.50% <ø> (ø)
bitswap/network/ipfs_impl.go 0.00% <ø> (ø)
bitswap/server/internal/decision/engine.go 76.05% <ø> (ø)
... and 18 more

... and 12 files with indirect coverage changes

@Jorropo
Copy link
Contributor

Jorropo commented May 30, 2023

ipfs/kubo#9787 (comment) 🤣 #309 has some merits I see

@aschmahmann aschmahmann force-pushed the feat/remove-ipld-legacy-and-format-globals branch from cca799c to 3200229 Compare June 7, 2023 19:17
@willscott willscott changed the title feat: remove ipld legacy and format globals feat: remove ipld legacy and format globals in ipld/merkledag Jun 7, 2023
@aschmahmann
Copy link
Contributor Author

merging without approval from the other kubo maintainers since I feel like @willscott's review is enough 😄. If someone objects we can fix it up before the boxo release tag.

@aschmahmann aschmahmann merged commit ade5ad0 into main Jun 7, 2023
@aschmahmann aschmahmann deleted the feat/remove-ipld-legacy-and-format-globals branch June 7, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants